Conversation
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
As mentioned in comment, swapping out these functions to suddenly use shndx doesn't seem right to me; adding the STB_WEAK check in the is_import function is good catch (though it now has me wondering whether all these functions should be marked private).
However, we can review adding a new api that uses the shndx in another patch if that is something people really want, or some other design that incorporates it somehow.
| /// Is this information defining an import? | ||
| #[inline] | ||
| pub fn is_import(info: u8, value: u64) -> bool { | ||
| pub fn is_import(info: u8, shndx: u64) -> bool { |
There was a problem hiding this comment.
idly wondering if we should de-pub this along with the other functions; i wonder why they were made pub, could have been a historical accident or perhaps someone uses them for whatever reason; i don't see bingrep for example using it anywhere
| pub fn is_import(info: u8, shndx: u64) -> bool { | ||
| let bind = st_bind(info); | ||
| bind == STB_GLOBAL && value == 0 | ||
| (bind == STB_GLOBAL || bind == STB_WEAK) && shndx == SHN_UNDEF as _ |
There was a problem hiding this comment.
this is a good catch (the missing STB_WEAK), but changing this function to operate semantically on shndx values instead of st_value, while backwards compatible, has wider implications.
for example, any clients relying on st_value being used in these computations will now break when section headers are stripped.
imho my general thesis that what a dynamic linker can import is how we've defined import, while arguably more nuanced than this, is a good one and has value (no pun intended). using st_value is a heuristic for that thesis.
that being said, it seems like people are hitting some issues with this, so we can consider adding something like is_import_from_sections or something which is new api to opt into using the section header (perhaps along with st_value) as the determinant.
|
closing this for now as per my explanation above; we can investigate adding the additional helper function if need be |
2 participants
Closes #288
Closes #419
I also found this bug on with ELF produced on illumos.
st_valuemay not be zero because it may indicate the offset of a import function.